Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvm/pr-subscribers-mlir-gpu

@llvm/pr-subscribers-mlir-spirv

Author: Jaeho Kim (oojahooo)

diff --git a/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp b/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp
index 5ecbd5d7c59d5..dbaa10e89bd42 100644
--- a/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp
@@ -184,6 +184,15 @@ spirv::TargetEnvAttr spirv::lookupTargetEnv(Operation *op) {
     if (!op)
       break;
 
+    if (auto arrAttr = op->getAttrOfType<ArrayAttr>("targets")) {
+      for (auto attr : arrAttr) {
+        if (auto spirvTargetEnvAttr =
+                llvm::dyn_cast<spirv::TargetEnvAttr>(attr)) {
+          return spirvTargetEnvAttr;
+        }
+      }
+    }
+
     if (auto attr = op->getAttrOfType<spirv::TargetEnvAttr>(
             spirv::getTargetEnvAttrName()))
       return attr;

@llvm/pr-subscribers-mlir

Author: Jaeho Kim (oojahooo)

diff --git a/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp b/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp
index 5ecbd5d7c59d5..dbaa10e89bd42 100644
--- a/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp
@@ -184,6 +184,15 @@ spirv::TargetEnvAttr spirv::lookupTargetEnv(Operation *op) {
     if (!op)
       break;
 
+    if (auto arrAttr = op->getAttrOfType<ArrayAttr>("targets")) {
+      for (auto attr : arrAttr) {
+        if (auto spirvTargetEnvAttr =
+                llvm::dyn_cast<spirv::TargetEnvAttr>(attr)) {
+          return spirvTargetEnvAttr;
+        }
+      }
+    }
+
     if (auto attr = op->getAttrOfType<spirv::TargetEnvAttr>(
             spirv::getTargetEnvAttrName()))
       return attr;

A rather naive question, as I'm not familiar with the GPU dialect. Is it possible for the target env to be present in both "targets" and as an op attribute at the same time? If so, does the look-up order matter? I’d hope the same target env is returned either way, but hypothetically if there is a different env in "targets" and in the op, I wonder which one should take a precedence?

Also, it'd be nice to have a test for the change. Probably in GPU to SPIR-V conversion test directory?

When the gpu.module op already has a target env as a dict-attr, running the spirv-attach-target pass can reproduce the situation you described. In such cases, I believe that the target env inserted by spirv-attach-target can be occurred without external intervention, so it should be given the highest priority during lookup. If that's the case, I think the flow in the committed code would be appropriate.

Also I added a lit test in GPU to SPIR-V conversion test directory. Thank you for your comment.

Could you add a test that shows where this comes up? I'm not sure if we should look up the target env in"targets" attribute in any of possible parents

Thank you for your comment. I added a test that reproduces the situation I had in mind.
If target env is inserted via the spirv-attach-target pass when no op—including builtin.module or gpu.module—has an existing one, then under the current implementation, the target env is attached only to the "targets" array attribute of gpu.module, and does not appear elsewhere.
When the convert-gpu-to-spirv pass is run in this case, it fails to find the target env in "targets" and instead falls back to the default target env, which leads to a legalization failure of the memref.load op.

If target env is inserted via the spirv-attach-target pass when no op—including builtin.module or gpu.module—has an existing one, then under the current implementation, the target env is attached only to the "targets" array attribute of gpu.module, and does not appear elsewhere.
When the convert-gpu-to-spirv pass is run in this case, it fails to find the target env in "targets" and instead falls back to the default target env, which leads to a legalization failure of the memref.load op.

To me the sounds like the wrong fix. Could we teach the passes you mentioned to also look in "targets" instead of relying on the core lookupTargetEnv helper that doesn't know anything about the gpu dialect?

If target env is inserted via the spirv-attach-target pass when no op—including builtin.module or gpu.module—has an existing one, then under the current implementation, the target env is attached only to the "targets" array attribute of gpu.module, and does not appear elsewhere.
When the convert-gpu-to-spirv pass is run in this case, it fails to find the target env in "targets" and instead falls back to the default target env, which leads to a legalization failure of the memref.load op.

To me the sounds like the wrong fix. Could we teach the passes you mentioned to also look in "targets" instead of relying on the core lookupTargetEnv helper that doesn't know anything about the gpu dialect?

If I understand your point correctly, rather than hardcoding the lookupTargetEnv helper to directly search for the "targets" attribute, which is specific to the gpu dialect, it would be more appropriate to handle this logic within the convert-gpu-to-spirv pass.

There are two possible approaches that I think:

1. As you suggested, we could modify the convert-gpu-to-spirv pass to explicitly inspect the "targets" attribute of gpu.module and retrieve the corresponding spirv.target_env. If that lookup fails, we can fall back to invoking the spirv::lookupTargetEnvOrDefault function. I believe this is a reasonable solution since the pass is already coupled with the gpu dialect by design.

2. Alternatively, we could generalize the logic of lookupTargetEnv to retrieve all attributes via getAttrDictionary() and recursively walk through them to locate a spirv.target_env attribute. While this approach differs slightly from your suggestion, I thought it could more robustly support the intended semantics of passes using lookupTargetEnvOrDefault, and would also remain dialect-agnostic.

I would greatly appreciate it if you could let me know which is aligned with your suggestion. If so, I would be happy to revise the implementation accordingly based on the preferred direction.

Thank you for your support.

Approach 1. sounds to me like the way to go

Approach 1. sounds to me like the way to go

Thank you for replying.
I reverted logic of lookupTargetEnv and added new helper function named lookupTargetEnvInTargets, which finds spirv target env in "targets", into GPUToSPIRV code.
Also I added same lookup logic in GPUModuleConversion for remaining target env during conversion.

Happy to make any further changes if needed. Otherwise, would it be okay to merge? Thank you!